pkg/network/ovn_kubernetes_test.go (1)

3951-3979: Add a same-value rerender case to verify hash stability.

The table only checks nil -> 0 -> 10. Add a repeated value case (for example, 10 again with a new pointer) and assert hashes do not change, so this test catches non-deterministic hash inputs.

Suggested test case addition

 		{
 			name:                                "Reachability timeout changed to 10",
 			reachabilityTimeout:                 ptrToUint32(10),
 			expectHashChanged:                   true,
 			expectKubernetesFeatureReachability: true,
 			expectErr:                           false,
 		},
+		{
+			name:                                "Reachability timeout remains 10",
+			reachabilityTimeout:                 ptrToUint32(10),
+			expectHashChanged:                   false,
+			expectKubernetesFeatureReachability: true,
+			expectErr:                           false,
+		},
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/network/ovn_kubernetes_test.go` around lines 3951 - 3979, Add a new test
case in the testCases slice used in ovn_kubernetes_test.go that repeats the same
value for reachabilityTimeout (e.g., use ptrToUint32(10) again with a freshly
allocated pointer) and set expectHashChanged to false and
expectKubernetesFeatureReachability to true; this ensures the code path that
computes the hash (referenced by the testCases slice, reachabilityTimeout, and
expectHashChanged) verifies stability across a rerender with identical logical
values even when pointer instances differ.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/network/ovn_kubernetes_test.go (1)

4034-4037: Avoid forcing HostedControlPlane in a generic reachability/hash rollout test

Line 4034-Line 4037 forces the HyperShift path, which can under-cover the default self-hosted rendering path this PR targets. Prefer default infra here, or add explicit table coverage for both hosted and self-hosted modes.

Minimal scope fix

-			// Set is as Hypershift hosted control plane.
-			bootstrapResult.Infra = bootstrap.InfraStatus{}
-			bootstrapResult.Infra.HostedControlPlane = &hypershift.HostedControlPlane{}
+			// Keep default self-hosted path in this test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/network/ovn_kubernetes_test.go` around lines 4034 - 4037, The test is
forcing the HyperShift path by setting bootstrapResult.Infra.HostedControlPlane,
which skips the default self-hosted rendering path; update the test so it leaves
bootstrapResult.Infra as the default (do not set HostedControlPlane) or convert
this case into table-driven tests that explicitly include both scenarios (one
case with bootstrapResult.Infra.HostedControlPlane set and one with it unset)
and assert renderOVNKubernetes outputs for each; locate the setup around
bootstrapResult.Infra and the call to renderOVNKubernetes to make this change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/network/ovn_kubernetes_test.go (2)

4024-4027: Add a self-hosted render pass for reachability timeout coverage.

The test currently forces HyperShift/managed rendering only. Since both managed and self-hosted control-plane templates were changed, add a non-hosted case to cover the self-hosted path as well.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/network/ovn_kubernetes_test.go` around lines 4024 - 4027, The test
currently only sets bootstrapResult.Infra.HostedControlPlane to a non-nil
hypershift.HostedControlPlane and calls renderOVNKubernetes, which skips the
self-hosted path; add an additional render pass where
bootstrapResult.Infra.HostedControlPlane is nil (or unset) to simulate a
self-hosted control plane and call renderOVNKubernetes(config, bootstrapResult,
manifestDirOvn, fakeClient, featureGatesCNO) again so the self-hosted templates
and reachability timeout path are exercised and asserted.

---

3955-3955: expectErr is dead in the current table.

All cases set expectErr: false, so the error branch is never exercised. Either add failing cases or remove the field/branch to keep the test focused.

Also applies to: 4028-4031

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/network/ovn_kubernetes_test.go` at line 3955, The table-driven test
contains an unused expectErr field (all entries set to false) so the error
branch is never exercised; either add one or more negative test rows with
expectErr: true to cover the error path or remove the expectErr field and the
conditional branch that checks it to simplify the test. Locate the table (the
variable containing entries with expectErr) and the code that branches on
expectErr in ovn_kubernetes_test.go, then update the table to include failing
cases that exercise the error behavior or delete the expectErr column and the
associated if/else error handling in the test loop to keep the test focused.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/network/ovn_kubernetes_test.go (1)

3951-4118: ⚠️ Potential issue | 🟠 Major

Missing regression guard for restart trigger (template/hash mutation).

The test validates rendering, but it still does not assert that ovnkube-node and ovnkube-control-plane pod templates mutate when timeout changes (and stay stable when unchanged). That is the core behavior this bug fix must protect.

Suggested hardening

 func TestRenderOVNKubernetesReachability(t *testing.T) {
 	g := NewGomegaWithT(t)
+	var prevNodeTemplate map[string]interface{}
+	var prevControlPlaneTemplate map[string]interface{}
+	var prevTimeout *uint32

-	for _, tc := range testCases {
+	for i, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			...
 			objs, _, err := renderOVNKubernetes(config, bootstrapResult, manifestDirOvn, fakeClient, featureGatesCNO)
 			...
+			renderedNode := findInObjs("apps", "DaemonSet", "ovnkube-node", "openshift-ovn-kubernetes", objs)
+			g.Expect(renderedNode).NotTo(BeNil())
+			nodeTemplate, found, err := uns.NestedMap(renderedNode.Object, "spec", "template")
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(found).To(BeTrue())
+
+			renderedControlPlane := findInObjs("apps", "Deployment", "ovnkube-control-plane", "openshift-ovn-kubernetes", objs)
+			g.Expect(renderedControlPlane).NotTo(BeNil())
+			controlPlaneTemplate, found, err := uns.NestedMap(renderedControlPlane.Object, "spec", "template")
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(found).To(BeTrue())
+
+			if i > 0 {
+				timeoutChanged := (prevTimeout == nil) != (tc.reachabilityTimeout == nil) ||
+					(prevTimeout != nil && tc.reachabilityTimeout != nil && *prevTimeout != *tc.reachabilityTimeout)
+				g.Expect(!reflect.DeepEqual(nodeTemplate, prevNodeTemplate)).To(Equal(timeoutChanged),
+					"ovnkube-node pod template mutation should track reachability timeout changes")
+				g.Expect(!reflect.DeepEqual(controlPlaneTemplate, prevControlPlaneTemplate)).To(Equal(timeoutChanged),
+					"ovnkube-control-plane pod template mutation should track reachability timeout changes")
+			}
+			prevNodeTemplate = nodeTemplate
+			prevControlPlaneTemplate = controlPlaneTemplate
+			prevTimeout = tc.reachabilityTimeout
+
 			if tc.expectKubernetesFeatureReachability {
 				...
 			}
 		})
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/network/ovn_kubernetes_test.go` around lines 3951 - 4118, The test lacks
assertions that the ovnkube-control-plane and ovnkube-node pod templates
actually mutate when the reachability timeout changes (and remain unchanged when
the timeout is unchanged); update the test to record the previous rendered
pod-template artifacts (use the extracted scriptCP and scriptNode strings or
compute a short hash of them) across iterations and then: when a test case
changes the timeout value from the previous case assert scriptCP/scriptNode have
changed, when the timeout is the same assert they are equal. Use the existing
identifiers (testCases, scriptCP, scriptNode, ovnkube-control-plane,
ovnkube-node) to locate where to capture and compare previous values and add
assertions for mutation vs stability accordingly.